Skip to content

feat: SFPG cross-rank completion — capabilities, ZBL bridging and spin+ZBL multi-rank - #5939

Open
wanghan-iapcm wants to merge 29 commits into
deepmodeling:masterfrom
wanghan-iapcm:feat-sfpg-multirank
Open

feat: SFPG cross-rank completion — capabilities, ZBL bridging and spin+ZBL multi-rank#5939
wanghan-iapcm wants to merge 29 commits into
deepmodeling:masterfrom
wanghan-iapcm:feat-sfpg-multirank

Conversation

@wanghan-iapcm

@wanghan-iapcm wanghan-iapcm commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Closes #5906.

The DPA4/SeZM Source Freeze Propagation Gate computes each node's eta_j = prod over outgoing edges of w(r_e); under MPI domain decomposition a rank only holds edges with owned destinations, so the src-keyed per-node partials are rank-incomplete and bridged models were single-rank only. This PR completes the gate across ranks and, as a prerequisite, promotes the export-time questions to atomic-model capabilities so compositions answer by aggregation.

Phase 1 — capability aggregation (issue Task 4)

  • Split the conflated descriptor capability: has_message_passing_across_ranks (needs the per-block exchange; unconditionally true for SeZM) vs the new supports_edge_parallel (can run under domain decomposition).
  • Six capabilities on BaseAtomicModel with concrete defaults, descriptor delegation on DPAtomicModel, and any/all aggregation on LinearEnergyAtomicModel: has_message_passing_across_ranks (any), supports_edge_parallel (all), dense_lower_supports_comm (all), uses_compact_edge_pairs (any), graph_edge_dtype (float32 iff all children), supports_graph_export (all).
  • forward_lower_graph_exportable_with_comm hoisted from EnergyModel into make_model (one owner, next to the non-comm twin) so LinearEnergyModel compositions can export it.
  • The four serialization.py helpers now consult the atomic model — no isinstance-on-concrete-model checks, no .descriptor walks. Regression fixed: a linear composition of two DPA2 children now gets its with-comm artifact (previously denied by wrapper type).
  • Composition-safe reach-ins outside serialization: .pt-checkpoint eval no longer crashes on compositions (ntypes via the model API), enable_compile degrades gracefully, and pt_expt get_standard_model honors bridging_method like its dpmodel twin (_compose_bridging is the single composition owner).

Phase 2 — SFPG cross-rank completion (issue Tasks 2 and 3)

No new communication machinery: the fix is one extra invocation of the existing deepmd_export::border_op_backward + border_op pair (they are exact transposes, R = B^T) on an (N, 2) [log_eta, zero_count] tensor before the gate is applied — reverse-accumulate ghost partials into owners, then broadcast the completed values back. Zero C++ changes.

  • border_op_backward gains autograd (its gradient is border_op's forward), so gate gradients cross ranks.
  • dpmodel: compute_edge_src_gate packs the partials through an optional node_partial_exchange hook; the dpmodel _gate_partial_exchange raises (single-process reference), the pt_expt subclass implements it on the border-op pair.
  • pt backend wired the same way. The red run of the new pt parity test demonstrated the issue's claim and more: pt's bridged parallel path did not just compute a silently wrong gate — it crashed outright (the ZBL injection indexed per-local types with extended ghost src indices); fixed by reading extended types.
  • Gates flipped: supports_edge_parallel is now True for bridged SeZM in both backends; bridged (and spin+ZBL) graph freezes embed the nested forward_lower_with_comm.pt2.

Verification

Anti-vacuous discipline throughout: every parity test places a sub-r_outer pair ACROSS the periodic/rank boundary (without it every cross-rank gate contribution is log w = 0), covers both bridging channels (hard-freeze zero_count at 0.4 Å, transition-zone log_eta at ~1 Å), and carries an identity-exchange ablation that must diverge.

  • Eager self-comm parity vs the folded reference at rtol/atol 1e-12 (energy, force, and force_mag for the spin variant), pt and pt_expt.
  • make_fx traces both border ops symbolically (21-input with-comm ABI unchanged); freeze embeds the nested artifact for ZBL and spin+ZBL compositions.
  • LAMMPS end-to-end on a Tesla T4: 2-rank vs 1-rank close-pair parity for ZBL (pair_style deepmd) and spin+ZBL (pair_style deepspin, incl. magnetic forces) — the spin+ZBL variant gets its first LAMMPS file. All 24 *Dpa4Zbl* C++ gtests pass (CPU + T4).
  • Variant-alignment coverage: ZBL empty-rank fail-fast twin, the first test of the DeepSpin owned-empty phantom path, charge-spin through pair_style deepspin, and default-CLI dp freeze resolution (nlist→graph auto-override + with-comm artifact) for both compositions.

Known limitations

  1. pt eager multi-rank bridging has no true-MPI pt test (no pt .pth LAMMPS ZBL fixtures exist); its parity rung is self-comm.
  2. graph_edge_dtype composition rule (float32 iff ALL children) is conservative; fp64 is the universal ABI.
  3. supports_graph_export keeps the hardcoded "cuda" probe inside pt_expt DPA1 (capability promoted; probe internals unchanged).
  4. The NativeSpinModelKind marker-base check in _needs_with_comm_artifact remains (a cross-backend family test, not a concrete-type reach-through).
  5. Model-deviation coverage stays absent for all dpa4 variants (pre-existing; Python model_devi has no spin support at all).
  6. DeepPot vs DeepSpin empty-rank designs deliberately differ (fail-fast vs phantom-pad, PR fix(cc): handle nloc==0 in DeepSpinPTExpt with phantom-atom padding #5485); both are now pinned per variant, not unified.
  7. Found while testing, left for a separate fix: source/api_c/include/deepmd.hpp uses &vec[0] on possibly-empty vectors (~33 sites) — undefined behavior that SIGABRTs under _GLIBCXX_ASSERTIONS before the empty-rank guard's message can fire (benign on non-hardened builds).

Summary by CodeRabbit

  • New Features

    • Added multi-rank inference for bridged DPA4/SeZM models, including native-spin and ZBL configurations.
    • Improved graph export detection, metadata, edge precision, and communication-aware export.
    • Added atomic-output-only inference for statistics workflows.
    • Standard model loading now preserves bridging configurations.
  • Bug Fixes

    • Improved handling of atom types, ghost atoms, empty MPI ranks, charge-spin inputs, and cross-rank calculations.
  • Documentation

    • Updated DPA4 and native-spin documentation for expanded multi-rank and graph export support.
  • Tests

    • Added regression coverage for MPI parity, graph exports, bridging, charge-spin behavior, and capability reporting.

Han Wang added 14 commits July 30, 2026 08:55
has_message_passing_across_ranks conflated the two; the SFPG bridging veto
moves to the new supports_edge_parallel() (issue deepmodeling#5906 Task 4 groundwork).
…ties

Six capabilities with concrete defaults on BaseAtomicModel, descriptor
delegation on DPAtomicModel, and any/all aggregation on
LinearEnergyAtomicModel (issue deepmodeling#5906 Task 4).
LinearEnergyModel compositions need it once the with-comm gate opens for
them (issue deepmodeling#5906 Task 4); one owner, next to the non-comm twin.
_translate_energy_keys moves along (make_model cannot import from
ener_model without a cycle); importers re-pointed.
_needs_with_comm_artifact / compact-edge-pairs / edge-dtype /
graph-export answer via the atomic model; two-DPA2 linear compositions
now get their with-comm artifact (issue deepmodeling#5906 Task 4).
.pt-checkpoint eval and enable_compile no longer reach for a single
descriptor; get_standard_model honors bridging_method like its dpmodel
twin, with _compose_bridging as the one composition owner (issue deepmodeling#5906
Task 4 audit findings).
R = B^T as linear maps over node rows, so the reverse-accumulate's
vector-Jacobian product is the forward broadcast (issue deepmodeling#5906: gate
gradients cross ranks).
compute_edge_src_gate packs (N,2) [log_eta, zero_count] through the
optional node_partial_exchange hook; zero_count becomes float so both
partials ride one border exchange. dpmodel's _gate_partial_exchange
raises (single-process reference); pt_expt overrides it (issue deepmodeling#5906).
_gate_partial_exchange = reverse-accumulate (border_op_backward) then
forward-broadcast (border_op) of the (N,2) [log_eta, zero_count]
partials. Eager self-comm parity vs the folded reference at 1e-12 with
a cross-boundary close pair in both bridging channels (hard-freeze and
transition zone); identity-exchange ablation diverges (issue deepmodeling#5906).
supports_edge_parallel flips to True (the SFPG exchange completes the
gate partials across ranks); the graph freeze embeds the nested
forward_lower_with_comm.pt2 for bridged compositions and gen_dpa4_zbl
asserts it (issue deepmodeling#5906 Task 2).
Closes the deepmodeling#5906 pt gap: the gate's per-node partials are completed via
border_op_backward + border_op before the gate is applied, and the
model-level ZBL injection reads EXTENDED types (the parallel path used
to crash on ghost src indices). Bridged SeZM reports
supports_edge_parallel=True; close-pair parity pinned on both bridging
channels with an identity-exchange ablation.
Replaces the fails-fast test: the with-comm artifact now completes the
SFPG partials across ranks, so a 2-rank run with the 0.9 A pair
straddling the processors 2 1 1 boundary must (and does) match the
1-rank reference (issue deepmodeling#5906 Task 2 E2E).
…ling#5906 Task 3)

The spin wrapper needed NO production change: the gate exchange sits
below it. gen_dpa4_spin_zbl asserts the with-comm artifact; python
self-comm parity (energy/force/force_mag, both bridging channels) at
1e-12; first-ever LAMMPS file for the spin+ZBL variant, incl. 2-rank
close-pair parity via pair_style deepspin.
Empty-rank behavior (zbl fail-fast twin; first test of the DeepSpin
phantom path -- owned-empty ranks with ghosts phantom-pad and match),
charge_spin via pair_deepspin, and dp-freeze default-CLI resolution for
zbl and native-spin compositions (issue deepmodeling#5906 Task 12b).
The SFPG per-node partials are completed across ranks by one
reverse-accumulate + forward-broadcast border exchange of an (N,2)
tensor per forward pass (issue deepmodeling#5906); also sweeps the stale
pre-deepmodeling#5884 native-spin single-rank claims.
@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds capability queries, cross-rank SFPG partial exchange for bridged DPA4/SeZM inference, capability-driven graph export, bridging-aware model composition, atomic-output statistics, training cleanup, and expanded PyTorch and LAMMPS validation.

Changes

SFPG and export pipeline

Layer / File(s) Summary
Capability contracts and aggregation
deepmd/dpmodel/atomic_model/*, deepmd/dpmodel/descriptor/*, deepmd/pt_expt/descriptor/*
Atomic models and descriptors expose distributed-inference, graph-edge dtype, dense communication, compact-edge, and graph-export capabilities, including composition aggregation rules.
Cross-rank SFPG partial exchange
deepmd/dpmodel/descriptor/dpa4.py, deepmd/dpmodel/descriptor/dpa4_nn/*, deepmd/pt/model/descriptor/*, deepmd/pt_expt/descriptor/dpa4.py
Bridged DPA4/SeZM paths pass node partials through edge-cache hooks and complete them across ranks using reverse accumulation and broadcast.
Model composition and graph export
deepmd/pt/model/model/*, deepmd/pt_expt/model/*, deepmd/pt_expt/utils/*, deepmd/pt_expt/infer/*
Model composition, atomic-only statistics, graph with-comm tracing, output translation, metadata, export eligibility, and border-operation autograd use the updated interfaces.
Training lifecycle and scheduling
deepmd/pt_expt/train/training.py
Training schedule resolution is centralized, distributed values are synchronized, attention warnings handle compositions, and owned data systems close in a finally block.
Capability, parity, and export validation
source/tests/common/*, source/tests/pt/*, source/tests/pt_expt/*, source/tests/infer/*
Tests cover capability aggregation, bridged parity, border-operation gradients, model composition, graph export, and with-comm artifact metadata.
LAMMPS MPI and spin integration
source/lmp/tests/*
LAMMPS tests cover charge-spin behavior, native-spin and ZBL parity across ranks, close-pair bridging, virials, and owned-empty rank handling.
DPA4 capability documentation
doc/model/dpa4.md
Documentation describes multi-rank ZBL bridging, native-spin graph archives, charge-spin conditioning, and with-comm artifacts.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related issues

Possibly related PRs

Suggested labels: C++

Suggested reviewers: iprozd, outisli, njzjz-bot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: SFPG cross-rank completion, capability interfaces, ZBL bridging, and spin-plus-ZBL multi-rank support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
⚔️ Resolve merge conflicts 💡
  • Resolve merge conflict in branch feat-sfpg-multirank
🛠️ Fix failing CI checks 💡
  • Fix failing CI checks
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread source/tests/pt/model/test_sezm_parallel_bridging_parity.py Fixed
Comment thread source/tests/pt/model/test_sezm_parallel_bridging_parity.py Fixed

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@doc/model/dpa4.md`:
- Around line 595-597: Update the native-spin support section heading in the
graph route documentation to use a positive statement, since the listed
multi-rank inference, charge-spin FiLM conditioning, and ZBL zone bridging
combinations are supported with spin.scheme: native.

In `@source/tests/infer/gen_dpa4_spin_zbl.py`:
- Around line 357-369: Update the _check_metadata docstring to state that it
verifies the presence of the with-comm artifact and the metadata flag, matching
the assertions for has_comm_artifact and forward_lower_with_comm.pt2. Remove the
contradictory wording that says it checks their absence.

In `@source/tests/pt/model/test_sezm_parallel_bridging_parity.py`:
- Around line 152-162: Update the parallel forward call to pass
sysm["edge_index"] as the graph connectivity argument where it currently repeats
sysm["edge_scatter_index"], matching the reference call while preserving
edge_scatter_index for its intended argument.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b6eb425d-2677-4159-a996-819c2ef72fc7

📥 Commits

Reviewing files that changed from the base of the PR and between 4f827cc and 57c25fe.

📒 Files selected for processing (40)
  • deepmd/dpmodel/atomic_model/base_atomic_model.py
  • deepmd/dpmodel/atomic_model/dp_atomic_model.py
  • deepmd/dpmodel/atomic_model/linear_atomic_model.py
  • deepmd/dpmodel/descriptor/dpa1.py
  • deepmd/dpmodel/descriptor/dpa2.py
  • deepmd/dpmodel/descriptor/dpa4.py
  • deepmd/dpmodel/descriptor/dpa4_nn/edge_cache.py
  • deepmd/dpmodel/descriptor/make_base_descriptor.py
  • deepmd/pt/model/descriptor/sezm.py
  • deepmd/pt/model/descriptor/sezm_nn/edge_cache.py
  • deepmd/pt/model/model/sezm_model.py
  • deepmd/pt_expt/descriptor/dpa1.py
  • deepmd/pt_expt/descriptor/dpa2.py
  • deepmd/pt_expt/descriptor/dpa4.py
  • deepmd/pt_expt/infer/deep_eval.py
  • deepmd/pt_expt/model/ener_model.py
  • deepmd/pt_expt/model/get_model.py
  • deepmd/pt_expt/model/make_model.py
  • deepmd/pt_expt/model/native_spin_model.py
  • deepmd/pt_expt/train/training.py
  • deepmd/pt_expt/utils/comm.py
  • deepmd/pt_expt/utils/serialization.py
  • doc/model/dpa4.md
  • source/lmp/tests/test_lammps_dpa4_chg_spin_deepspin_pt2.py
  • source/lmp/tests/test_lammps_dpa4_spin_graph_pt2.py
  • source/lmp/tests/test_lammps_dpa4_spin_zbl_pt2.py
  • source/lmp/tests/test_lammps_dpa4_zbl_pt2.py
  • source/tests/common/dpmodel/test_atomic_model_capabilities.py
  • source/tests/common/dpmodel/test_descrpt_dpa4.py
  • source/tests/infer/gen_dpa4_spin_zbl.py
  • source/tests/infer/gen_dpa4_zbl.py
  • source/tests/pt/model/test_sezm_parallel.py
  • source/tests/pt/model/test_sezm_parallel_bridging_parity.py
  • source/tests/pt_expt/model/test_dpa4_zbl_parallel.py
  • source/tests/pt_expt/model/test_export_with_comm.py
  • source/tests/pt_expt/model/test_get_model_bridging.py
  • source/tests/pt_expt/model/test_zbl_bridging.py
  • source/tests/pt_expt/test_dp_freeze.py
  • source/tests/pt_expt/utils/test_border_op_backward.py
  • source/tests/pt_expt/utils/test_graph_pt2_metadata.py
👮 Files not reviewed due to content moderation or server errors (11)
  • deepmd/dpmodel/descriptor/dpa4_nn/edge_cache.py
  • deepmd/pt/model/descriptor/sezm.py
  • deepmd/pt/model/descriptor/sezm_nn/edge_cache.py
  • deepmd/pt/model/model/sezm_model.py
  • deepmd/pt_expt/descriptor/dpa4.py
  • deepmd/pt_expt/infer/deep_eval.py
  • deepmd/pt_expt/model/ener_model.py
  • deepmd/pt_expt/model/get_model.py
  • deepmd/pt_expt/model/make_model.py
  • deepmd/pt_expt/model/native_spin_model.py
  • deepmd/pt_expt/utils/comm.py

Comment thread doc/model/dpa4.md Outdated
Comment thread source/tests/infer/gen_dpa4_spin_zbl.py
Comment thread source/tests/pt/model/test_sezm_parallel_bridging_parity.py
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.75776% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.35%. Comparing base (9b2582f) to head (285e3c6).
⚠️ Report is 18 commits behind head on master.

Files with missing lines Patch % Lines
deepmd/dpmodel/descriptor/dpa1.py 71.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5939      +/-   ##
==========================================
- Coverage   79.41%   79.35%   -0.07%     
==========================================
  Files        1072     1081       +9     
  Lines      124893   126242    +1349     
  Branches     4531     4592      +61     
==========================================
+ Hits        99187   100178     +991     
- Misses      24085    24411     +326     
- Partials     1621     1653      +32     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Han Wang added 3 commits July 31, 2026 09:17
Resolutions: keep the capability-based _needs_with_comm_artifact and the
_compose_bridging owner (renamed to upstream's InnerPotentialAtomicModel,
deepmodeling#5910); drop this branch's pt extended-atype ZBL fix in favor of
upstream's equivalent descriptor_atype at the relocated injection site.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
deepmd/pt_expt/train/training.py (1)

2147-2176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep legacy epoch lengths consistent with pt semantics.

pt_expt divides compute_total_numb_batch(...) by world_size for legacy data, while pt only uses the sharded LMDB dataloader length and otherwise returns the full weighted batch count. If legacy DeepmdDataSystem remains replicated per rank, this makes num_steps per epoch differ between backends for the same numb_epoch/num_epoch_dict configuration, changing effective epoch length and the derived LR schedule. Align legacy pt_expt with pt unless this parallel epoch definition is intentional.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deepmd/pt_expt/train/training.py` around lines 2147 - 2176, The _epoch_length
method currently divides every dataset’s weighted batch count by world_size,
unlike pt semantics for legacy DeepmdDataSystem data. Preserve the world_size
division only for sharded LMDB data, and return the full
compute_total_numb_batch result for replicated legacy data so epoch lengths and
LR schedules remain consistent across backends.
🧹 Nitpick comments (1)
deepmd/pt_expt/model/get_model.py (1)

246-249: 📐 Maintainability & Code Quality | 🔵 Trivial

Duplicated inner-clamp radii injection.

The 2-line inner-clamp radii injection (inner_clamp_r_inner/inner_clamp_r_outer from bridging_r_inner/bridging_r_outer) is repeated verbatim in get_sezm_model (lines 127-128) and here. Consider extracting a small helper (e.g. _inject_bridging_radii(data)) shared by both builders to avoid future divergence.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deepmd/pt_expt/model/get_model.py` around lines 246 - 249, Extract the
duplicated descriptor-radii assignment into a shared helper such as
_inject_bridging_radii(data), preserving the existing default values and
descriptor initialization. Replace the inline logic in both get_sezm_model and
the shown bridging_enabled path with calls to that helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@deepmd/pt_expt/train/training.py`:
- Around line 2147-2176: The _epoch_length method currently divides every
dataset’s weighted batch count by world_size, unlike pt semantics for legacy
DeepmdDataSystem data. Preserve the world_size division only for sharded LMDB
data, and return the full compute_total_numb_batch result for replicated legacy
data so epoch lengths and LR schedules remain consistent across backends.

---

Nitpick comments:
In `@deepmd/pt_expt/model/get_model.py`:
- Around line 246-249: Extract the duplicated descriptor-radii assignment into a
shared helper such as _inject_bridging_radii(data), preserving the existing
default values and descriptor initialization. Replace the inline logic in both
get_sezm_model and the shown bridging_enabled path with calls to that helper.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 50f94545-ef5a-4f70-939d-8fa0ac908f8a

📥 Commits

Reviewing files that changed from the base of the PR and between 57c25fe and 17136a8.

📒 Files selected for processing (9)
  • deepmd/pt/model/model/sezm_model.py
  • deepmd/pt_expt/model/get_model.py
  • deepmd/pt_expt/train/training.py
  • source/lmp/tests/test_lammps_dpa4_zbl_pt2.py
  • source/tests/common/dpmodel/test_atomic_model_capabilities.py
  • source/tests/infer/gen_dpa4_spin_zbl.py
  • source/tests/infer/gen_dpa4_zbl.py
  • source/tests/pt_expt/model/test_get_model_bridging.py
  • source/tests/pt_expt/model/test_zbl_bridging.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • source/tests/infer/gen_dpa4_zbl.py
  • source/tests/pt_expt/model/test_get_model_bridging.py
  • source/lmp/tests/test_lammps_dpa4_zbl_pt2.py

Two GitHub Advanced Security findings on the new file:

- 'unittest' was imported both as a module and via 'from unittest
  import mock'; use a plain 'import unittest.mock' instead.
- the 'deepmd.pt.utils.env' import was dead: this test pins CPU
  explicitly, and env is pulled in transitively by get_model plus the
  source/tests/pt package __init__.

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the DPA4/SeZM factory documentation to match the capabilities implemented by this PR.

Comment thread deepmd/pt_expt/model/get_model.py

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the stale single-rank limitation from the spin+ZBL fixture documentation now that this fixture pins the with-comm multi-rank path.

Comment thread source/tests/infer/gen_dpa4_spin_zbl.py

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the PT SeZM capability documentation consistent with the bridging multi-rank behavior implemented by this PR.

Comment thread deepmd/pt/model/descriptor/sezm.py Outdated

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align the DPA4 freeze-route documentation with the public pt_expt freeze behavior that always selects graph for graph-capable models.

Comment thread doc/model/dpa4.md

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the shared native-spin marker documentation to match the graph with-comm contract implemented here.

Comment thread deepmd/pt_expt/utils/serialization.py

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the new standard + DPA4 bridging route a fully supported, normalized equivalent of type: dpa4, rather than a direct-builder-only partial path.

Comment thread deepmd/pt_expt/model/get_model.py
Han Wang added 4 commits July 31, 2026 18:42
A DescrptHybrid inherited the concrete `dense_lower_supports_comm() ==
True` default while aggregating `has_message_passing_across_ranks()` over
its children. For hybrid([dpa4, se_e2_a]) that combination makes
`_needs_with_comm_artifact(model, "nlist")` true, so an auto/nlist freeze
builds a dense with-comm trace that dies in DPA4's NotImplementedError as
soon as comm_dict is passed.

Aggregate it (and supports_edge_parallel) with ALL, matching the veto
semantics already used for the atomic-model composition.
Two executable assertions still pinned the pre-deepmodeling#5906 single-rank
contract (`has_comm_artifact is False`) for the ZBL and native-spin+ZBL
graph freezes. They are skipped only when CI=true, so a local full-suite
run compiled the archive and then failed on the obsolete expectation.
Flip both to the multi-rank contract and also assert the nested
forward_lower_with_comm.pt2 entry is present.
make_model() produces DOS, dipole, polar, property and energy models, but
forward_lower_graph_exportable_with_comm hard-codes do_grad_r("energy"),
do_grad_c("energy") and _translate_energy_keys. Defining it on the common
factory made every non-energy model advertise an API that would raise on
the missing energy output; the serializer's energy gate only hid it.

Give it the same ownership as its non-comm twin: define it on EnergyModel
and alias it in LinearEnergyModel. NativeSpinEnergyModel is built on
EnergyModel, so the spin route keeps it through the MRO.
Six sites still described the pre-deepmodeling#5906 contract or a CLI behaviour that
does not exist:

- dpa4.uses_graph_lower / sezm.has_message_passing_across_ranks claimed
  bridging still fails multi-rank fast, contradicting the capability
  split this branch introduces.
- NativeSpinModelKind cited "native-spin lowers are single-rank only" as
  its motivating example; only the DENSE lower is excluded now.
- get_sezm_model advertised bridging and spin as unsupported although the
  factory deliberately constructs both; replace with the precise matrix
  (deepspin scheme, lora, use_compile, preset_out_bias remain unsupported).
- doc/model/dpa4.md offered plain DPA4 a choice of dense or graph freeze
  via a '--lower-kind auto' default. There is no 'auto' choice (nlist and
  graph only, default nlist) and freeze() overrides any non-graph request
  to graph for graph-capable models.
- gen_dpa4_spin_zbl.py still headed its contract 'Single-rank only'.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@deepmd/dpmodel/model/native_spin_model.py`:
- Line 37: Update the docstring near the shared cross-backend marker to remove
the duplicated word, changing “cross-backend family test test” to a single
“test” while preserving the rest of the marker wording.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2213b926-20f0-40ce-a83d-12c1285fa49b

📥 Commits

Reviewing files that changed from the base of the PR and between deabcb7 and 4021124.

📒 Files selected for processing (12)
  • deepmd/dpmodel/descriptor/dpa4.py
  • deepmd/dpmodel/descriptor/hybrid.py
  • deepmd/dpmodel/model/native_spin_model.py
  • deepmd/pt/model/descriptor/sezm.py
  • deepmd/pt_expt/model/dp_linear_model.py
  • deepmd/pt_expt/model/ener_model.py
  • deepmd/pt_expt/model/get_model.py
  • deepmd/pt_expt/model/make_model.py
  • doc/model/dpa4.md
  • source/tests/common/dpmodel/test_atomic_model_capabilities.py
  • source/tests/infer/gen_dpa4_spin_zbl.py
  • source/tests/pt_expt/model/test_zbl_bridging.py
💤 Files with no reviewable changes (1)
  • deepmd/pt_expt/model/make_model.py
🚧 Files skipped from review as they are similar to previous changes (6)
  • deepmd/pt_expt/model/ener_model.py
  • doc/model/dpa4.md
  • deepmd/dpmodel/descriptor/dpa4.py
  • deepmd/pt/model/descriptor/sezm.py
  • source/tests/infer/gen_dpa4_spin_zbl.py
  • source/tests/pt_expt/model/test_zbl_bridging.py

Comment thread deepmd/dpmodel/model/native_spin_model.py Outdated
@wanghan-iapcm
wanghan-iapcm requested a review from OutisLi July 31, 2026 11:43
Han Wang added 2 commits July 31, 2026 19:45
Left over from removing the stale parenthetical in 4021124.
Conflict in deepmd/pt_expt/utils/serialization.py: upstream deepmodeling#5913
relocated the graph edge-dtype helper to deepmd/pt_expt/model/graph_lower.py
as a public graph_edge_dtype(), while this branch had rewritten the same
helper in place to answer from the atomic-model capability instead of
reaching through a single .descriptor.

Resolution keeps BOTH: upstream's location (serialization.py imports it
from graph_lower) with this branch's implementation. The reach-in version
is wrong for compositions -- a LinearEnergyAtomicModel has no .descriptor,
so a bridged/ZBL model would silently report float64 regardless of its
children -- which is exactly what the capability aggregation fixes.
Dropped the now-unused torch import from graph_lower.py.
get_standard_model honored `bridging_method` and returned a
LinearEnergyModel -- the type asked for was not the type returned. It also
made a second owner of the bridging build, and the two owners had already
drifted: get_sezm_model promotes descriptor.exclude_types to model-level
pair_exclude_types and this one never did, changing a 0.9 A Ni-O dimer by
79.97 eV (max |dF| 318.48 eV/A).

Reject `bridging_method` in the standard builder instead. Rejecting rather
than ignoring keeps the original fail-loud property: silently dropping the
term yields a physically different model than the config requests. DPA4
(`type: "dpa4"`/`"sezm"`) is now the single bridging owner in this backend.

The route was unreachable from a validated input.json anyway -- argcheck
declares `bridging_method` only under the dpa4/sezm variant -- so no
supported configuration changes behavior.

Follow-ups: deepmodeling#5947 (drop the exclusion promotion), deepmodeling#5948 (express bridging
as an explicit linear_ener composition, after which the restriction is
moot).

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PT freeze comment still describes bridged SeZM as single-rank.

deepmd/pt/entrypoints/freeze_pt2.py:951-956 says that bridging models report supports_edge_parallel() == False, that Source Freeze Propagation is not rank-decomposable, and that they fall back to single-rank. That is now the opposite of the contract implemented by this PR: bridged SeZM reports True, _gate_partial_exchange() completes the SFPG partials across ranks, and the surrounding predicate therefore embeds forward_lower_with_comm.pt2 for the edge_vec route. The new PT capability tests also pin that behavior.

Please update this production comment together with the capability change. Leaving the explanation inverted next to the artifact-selection predicate makes the multi-rank contract particularly easy to misread during future maintenance.

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DPA4 documentation makes an unsupported performance claim about the new SFPG communication path.

Comment thread doc/model/dpa4.md Outdated

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactored DPA4/SeZM factory now has an internally inconsistent return-type contract.

Comment thread deepmd/pt_expt/model/get_model.py Outdated
Han Wang added 2 commits August 4, 2026 17:43
- get_sezm_model was annotated `-> EnergyModel`, but its bridging path
  returns LinearEnergyModel, which is NOT an EnergyModel. Annotate it and
  get_standard_model against BaseModel (the honest common contract, as pt
  already does) and give _compose_bridging its concrete LinearEnergyModel
  result, instead of erasing all three with `Any`.
- pt/entrypoints/freeze_pt2.py still explained the with-comm predicate by
  saying bridging reports supports_edge_parallel()=False and falls back to
  single-rank -- inverted since this branch.
- doc/model/dpa4.md called the SFPG exchange "negligible". Both kernels end
  in an MPI_Barrier and the force graph differentiates through them, so the
  transposes add a matching pair of round trips; at small system sizes or
  high rank counts that can dominate. It has not been benchmarked, so the
  claim is removed rather than restated.
test_lammps_dpa4_spin_zbl_pt2.py shared 421 byte-identical lines with
test_lammps_dpa4_spin_graph_pt2.py: the 4-atom NiO system, the live DeepPot
reference, setup/teardown, LAMMPS construction, force_mag gathering, the
MPI subprocess/parser and the parity assertions. Any change to tolerances,
the runner output schema or timeout handling had to be made twice.

Extract them into dpa4_spin_harness.py. Each module now carries only its
archive, its geometry and its variant-specific claims:

  spin_graph  605 -> 230 lines
  spin_zbl    566 -> 214 lines

Kept narrowly scoped rather than folded into lammps_test_utils: these
pieces are DPA4-native-spin-specific (`atom_modify map yes` for the graph
route's atom map, the spin_norm/hbar `fm` convention, and a timeout-bounded
runner that SIGKILLs the whole mpirun process group), and moving them into
the shared module would change behaviour for unrelated tests.

Same 8 tests collect with the same IDs; the graph module's two MPI tests
now use the shared assertion helper the ZBL module had already factored
out. Runtime validation is the C++/LAMMPS CI jobs: the .pt2 fixtures are
not present locally and this box has no reliable MPI.
@wanghan-iapcm

Copy link
Copy Markdown
Collaborator Author

@OutisLi — the PT freeze comment is fixed in 2e7865c. deepmd/pt/entrypoints/freeze_pt2.py no longer says bridging reports supports_edge_parallel()=False and falls back to single-rank; it now states that bridging models DO take the with-comm path because the SFPG per-node partials are completed across ranks by DescrptSeZM._gate_partial_exchange, leaving only virtual spin (the nlist interface) excluded. You were right that leaving the explanation inverted next to the artifact-selection predicate was the dangerous part.

All four points from your 2026-08-01/08-02 rounds are now addressed:

thread resolution
PT freeze comment fixed, 2e7865c
"negligible" SFPG claim claim removed, 2e7865c
factory return contract typed against BaseModel, 2e7865c
LAMMPS harness duplication extracted, 0408b38
SFPG graph fast path tracked as #5957 (your option 2)

Two follow-ups from the earlier round remain filed rather than fixed here: #5947 (drop the descriptor.exclude_types promotion) and #5948 (express bridging as an explicit linear_ener composition, which subsumes #5947).

@wanghan-iapcm
wanghan-iapcm requested a review from OutisLi August 4, 2026 10:21
Comment thread deepmd/pt_expt/model/get_model.py
# phantom path and none trips the genuinely-empty fail-fast. The shifted
# coordinates (``coord_empty_rank``) are defined below, after ``coord``.
data_file_empty_rank = Path(__file__).parent / "data_dpa4_spin_graph_pt2_empty_rank.lmp"
data_file_empty_rank = Path(__file__).parent / "data_dpa4_spin_graph_pt2_empty_rank.lmp"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Remove the duplicated fixture-path assignment

The refactor introduces this exact data_file_empty_rank assignment twice in succession. It is a runtime no-op, but it is an obvious patch artifact in a commit whose purpose is to establish a clean single-owner test harness. Please keep a single definition.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 285e3c6 — you read it correctly, it was a patch artifact. My extraction script preserved the original comment block by line range, and that range already ended with the data_file_empty_rank assignment, while the rewrite template emitted it again. Single definition now; collection still yields the same 4 test IDs for that module.

- The harness extraction emitted `data_file_empty_rank` twice in a row: the
  preserved comment block already carried the assignment and the rewrite
  template added it again. A runtime no-op, but a patch artifact in the
  commit whose point was a clean single-owner harness.
- TestNativeSpinWithBridging still described `get_standard_model` as the
  owner of the bridging composition, which is the opposite of the contract
  this branch establishes: that builder now rejects `bridging_method`, and
  `get_native_spin_model` routes DPA4/SeZM to `get_sezm_model` and
  re-classes the composition it returns.
@wanghan-iapcm
wanghan-iapcm requested a review from OutisLi August 4, 2026 14:23

@OutisLi OutisLi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the latest HEAD. The previously raised correctness, performance-documentation, type-contract, and code-structure issues have been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DPA4/SeZM zone-bridging (SFPG) gate is incomplete under domain decomposition; blocks ZBL and spin+ZBL multi-rank

3 participants